Skip to content

[DWDS] Make pause a no-op on WebSocketProxyService #2697

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 13, 2025
Merged

[DWDS] Make pause a no-op on WebSocketProxyService #2697

merged 1 commit into from
Oct 13, 2025

Conversation

srujzs
Copy link
Contributor

@srujzs srujzs commented Oct 9, 2025

We can't actually pause during execution so we shouldn't signal that we did.

@srujzs
[DWDS] Make pause a no-op on WebSocketProxyService
a52e6c8 
We can't actually pause during execution so we shouldn't
signal that we did.
@srujzs srujzs requested review from bkonyi and jyameo October 9, 2025 23:05
@srujzs
Copy link
Contributor Author

srujzs commented Oct 9, 2025

I tried removing _currentPauseEvent entirely, but it looks like Flutter tools' testing is relying on a pause event at start, specifically from here when the isolate is created and pauseIsolatesOnStart is true. For now, I left that, modifications to that flag which also results in a new pause event being sent, and resume, where a resume event is sent.

It's not clear to me whether PauseStart and Resume should still be sent even though we don't pause. I suppose it signals on start that the isolate has started but we haven't run main yet?

@jyameo
Copy link
Contributor

jyameo commented Oct 10, 2025

I tried removing _currentPauseEvent entirely, but it looks like Flutter tools' testing is relying on a pause event at start, specifically from here when the isolate is created and pauseIsolatesOnStart is true. For now, I left that, modifications to that flag which also results in a new pause event being sent, and resume, where a resume event is sent.

It's not clear to me whether PauseStart and Resume should still be sent even though we don't pause. I suppose it signals on start that the isolate has started but we haven't run main yet?

It makes sense to me to keep the pause event when the isolate is created to maintain the expected behavior from Flutter tools.

jyameo
jyameo approved these changes Oct 10, 2025
View reviewed changes
Copy link
Contributor

@jyameo jyameo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@bkonyi
Copy link
Collaborator

bkonyi commented Oct 10, 2025

I tried removing _currentPauseEvent entirely, but it looks like Flutter tools' testing is relying on a pause event at start, specifically from here when the isolate is created and pauseIsolatesOnStart is true.

Can you point me to the failing test? I don't think there's any particular reason that the tool needs this event, so it could just be a test expectation that can be removed.

@srujzs
Copy link
Contributor Author

srujzs commented Oct 13, 2025

Can you point me to the failing test? I don't think there's any particular reason that the tool needs this event, so it could just be a test expectation that can be removed.

It's in the test driver itself: https://github.com/flutter/flutter/blob/0205d4e8cc24511918e0348410f34605102682c9/packages/flutter_tools/test/integration.shard/test_driver.dart#L295

I think it makes sense that it expects the event when the isolate starts, although perhaps it should be guarded by a conditional. I also think the WebSocketProxyService can handle it (as we can just wait to run main), so it makes sense to me to support it. I can upload a follow-up PR if we think we want to change that in DWDS.

@srujzs srujzs merged commit 2517aa9 into dart-lang:main Oct 13, 2025
47 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Oct 16, 2025
@devoncarew
[deps] rev ecosystem, http, tools, webdev
a833d29 
Revisions updated by `dart tools/rev_sdk_deps.dart`.

ecosystem (https://github.com/dart-lang/ecosystem/compare/f4fbac4..6a8746e):
  6a8746e  2025-10-13  Moritz  Use newest verion of firehose in publish (dart-lang/ecosystem#378)

http (https://github.com/dart-lang/http/compare/2c53fa3..2cb6c12):
  2cb6c12  2025-10-15  Brian Quinlan  chore(cronet_http): Prepare to release 1.6.0 (dart-lang/http#1833)
  a906598  2025-10-16  Denys Vitali  fix(ok_http): trust manager init (dart-lang/http#1830)
  54dfd63  2025-10-15  Brian Quinlan  docs(cupertino_http): Clarify that happens when a request is cancelled (dart-lang/http#1834)
  3e3c366  2025-10-15  Brian Quinlan  chore(okhttp): Fix CI by setting minSdk=24 (dart-lang/http#1835)
  6271faf  2025-10-13  Benjamin  feat: export MediaType from http_parser (dart-lang/http#1803)
  32dee0c  2025-10-13  Benjamin  chore(cronet_http): bump compileSdkVersion to 34 (dart-lang/http#1832)

tools (https://github.com/dart-lang/tools/compare/adf3fe7..f5920a2):
  f5920a2d  2025-10-16  Morgan :)  Fix `PollingDirectoryWatcher` for delete during poll. (dart-lang/tools#2212)
  289ba0d3  2025-10-15  Morgan :)  Add DirectoryWatcher symlink tests. (dart-lang/tools#2206)
  1275d4c3  2025-10-14  Kevin Moore  Bump deps for graphs, html, process, pubspec_parse (dart-lang/tools#2210)
  8b9a48ca  2025-10-14  Morgan :)  Fix test handling of link timestamps. (dart-lang/tools#2208)
  81588f32  2025-10-14  Abgar Simonean  Update to oauth2 package README for Flutter Web info (dart-lang/tools#1961)

webdev (https://github.com/dart-lang/webdev/compare/186bfe7..2517aa9):
  2517aa94  2025-10-13  Srujan Gaddam  [DWDS] Make pause a no-op on WebSocketProxyService (dart-lang/webdev#2697)

Change-Id: Ifee5e199cf5799aa5962639e686192874f4d58b3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/455480
Auto-Submit: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jyameo jyameo jyameo approved these changes

@bkonyi bkonyi Awaiting requested review from bkonyi

Assignees

No one assigned

Labels

package:dwds

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@srujzs @jyameo @bkonyi